Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid warning on workflow_call triggers #2274

Merged
merged 1 commit into from
May 8, 2024

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented May 6, 2024

Typically, we warn when there is no push trigger in the workflow file that triggered this run. However, when this action is triggered by a workflow_call event, we assume there is a custom process for triggering the action and we don't want to warn in this case.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@aeisenberg aeisenberg requested a review from a team as a code owner May 6, 2024 22:28
src/workflow.ts Fixed Show fixed Hide fixed
@aeisenberg aeisenberg force-pushed the aeisenberg/no-warn-workflow_call branch from 6527f17 to 9a8503f Compare May 6, 2024 22:42
src/workflow.ts Outdated
// Avoid warning when there this action was triggered via workflow_call since
// the user has a custom workflow that is calling this action and we assume
// they know what they are doing.
const isWorkflowCall = github.context.eventName === "workflow_call";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain this will work. If you're in a callee workflow, we only see the name of the event that started the caller workflow. I think you'll have to use other heuristics to parse the current workflow (which will be the caller) to recognise that it contains a workflow call, or simply doesn't contain a github/codeql-action step (which indicates that step is in the callee).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Aditya is right, and we want to look for a workflow_call trigger, i.e. workflow_call appearing in the on property (note that this property may be a string or an array).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just need to look for the presence of the property, IIUC. The value can also be null. Eg, this is legal:

on:
  workflow_call:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does on: workflow_call get parsed? I assumed it was a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is parsed as null:
YAML_to_JSON_Converter_Online

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah...I see you were saying something different. We need to handle the following formats as well:

on: workflow_call

on:
   - workflow_call

@aeisenberg aeisenberg force-pushed the aeisenberg/no-warn-workflow_call branch from 9a8503f to 43d7704 Compare May 7, 2024 18:11
src/workflow.ts Fixed Show fixed Hide fixed
@aeisenberg aeisenberg force-pushed the aeisenberg/no-warn-workflow_call branch from 43d7704 to ee0b233 Compare May 7, 2024 18:18
@aeisenberg
Copy link
Contributor Author

I implemented the suggested changes, made a refactoring to make things a bit easier to understand, and added a handful of tests to verify.

Typically, we warn when there is no `push` trigger in the
workflow file that triggered this run. However, when this
action is triggered by a `workflow_call` event, we assume
there is a custom process for triggering the action and we
don't want to warn in this case.
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

return doc.on.includes(triggerName);
}

return Object.prototype.hasOwnProperty.call(doc.on, triggerName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify this:

Suggested change
return Object.prototype.hasOwnProperty.call(doc.on, triggerName);
return Object.hasOwn(doc.on, triggerName);

Copy link
Contributor Author

@aeisenberg aeisenberg May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I didn't know about this function.

It's only available on node 16.9 and later. Are we sure that all supported GHES runners use that version or later?

Also, we need to update our tsconfig, or else we ge tlinter errors. So, I'm going to hold off for now and we can make the tsconfig changes later.

@aeisenberg aeisenberg merged commit 7d9b7a1 into main May 8, 2024
320 checks passed
@aeisenberg aeisenberg deleted the aeisenberg/no-warn-workflow_call branch May 8, 2024 18:43
@github-actions github-actions bot mentioned this pull request May 13, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants